Add StoredVars tab #220
Add StoredVars tab #220
Conversation
|
I modified that you mentioned. |
There was a problem hiding this comment.
Some general feedback:
- I have to click twice to change between the variable name to it's value, once to blur the previous input, and another to focus the next, instead of simply moving between them.
- Hitting tab won't switch between the variable name and value inputs.
- Hitting tab also locks you inside the variable editing and won't let you reach other elements.
| <strong className="value">Value</strong> | ||
| <div className="deleteBtn"/> | ||
| </li>} | ||
| {variables.storedVars.entries().sort().map((storedMap) => ( |
There was a problem hiding this comment.
Keep them sorted via a computed property, either in the store, or in the component.
| </li>} | ||
| {variables.storedVars.entries().sort().map((storedMap) => ( | ||
| <Variable | ||
| key={Math.random()} |
There was a problem hiding this comment.
This beats the purpose of having a key in the first place, the idea is to have a persistent key throughout renders.
You can use the variable name as a key, since it is unique.
| @@ -36,6 +37,10 @@ class Variables { | |||
| @action.bound clearVariables() { | |||
| this.storedVars.clear(); | |||
| } | |||
|
|
|||
| @computed get isStop(){ | |||
There was a problem hiding this comment.
This is component specific logic, and has nothing to do with the general behavior of the variables store.
| return ( | ||
| <li className="variable"> | ||
| {this.props.isAdding ? | ||
| <input |
There was a problem hiding this comment.
What's the reason to use the default input element, and not the Input component?
| this.input.focus(); | ||
| } | ||
| handleKeyDown(e) { | ||
| if (e.key === "Enter") { |
There was a problem hiding this comment.
Use a form's onSubmit event, instead of keydown and checking for the key enter.
It is bypassing the browser default behavior.
| } | ||
| } | ||
| handleChanged() { | ||
| const isValidKey = this.state.key || this.props.keyVar; |
There was a problem hiding this comment.
The naming convention is misleading, variables that begin with is are booleans, while these are strings.
Perhaps call them validKey?
| } | ||
| handleChanged() { | ||
| const isValidKey = this.state.key || this.props.keyVar; | ||
| const isValidValue = this.state.value || this.props.value; |
There was a problem hiding this comment.
Why is empty string considered invalid? users can definitely store an empty string.
| } | ||
| addVariableClicked(isAdding) { | ||
| this.setState({ | ||
| isAddingVariable: isAdding |
There was a problem hiding this comment.
I don't like this state, it is also inconsistent with the experience we give with the command table,
let's just have a "pristine" variable line, just like in the command table.
|
@corevo Could you check it out? |
|
I was sick the past few weeks, I'll make sure to have a look. |
|
Hey, I've given this some thought, and theres a design issue we should discuss, wanna ping my on irc, or something. |
|
I don't care what happens to this pr, even though this pr was deleted. |
|
Do you mind if I take ownership? |
|
@corevo Sure, why not? |
|
👍 |
|
Hi @corevo ! I would like to know what happened to this PR? What is the status? and is someone working on it? I think it is a great feature to be implemented in the extension. |
|
@cdifino the problem is typings, since We need to support specific forms for specific variable types as to avoid casting them to strings. |
|
|
I need the
storedVartab before reference tab.